-
-
Notifications
You must be signed in to change notification settings - Fork 325
Documentation Rewrite #519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Btw if you think it's beneficial, feel free to request my review for any IDOM PRs. |
Here's the beta for the new react docs: https://beta.reactjs.org/ |
After doing a bunch of development with IDOM today, here's some readability suggestions for IDOM docs code snippets:
|
This isn't made clear in the docs anywhere, but I use "title-case" for those functions because that's how I identify that those functions define a component. With that convention, Using title case isn't actually the convention in JS either. Only component functions have title-cased names when developing with React and this fact is leveraged in the same way into a linter. |
59b1921
to
0eb17f3
Compare
@Archmonger I think the doc rewrite is going really well. Here's a build of the docs as of writing this: build.zip Hopefully you can use that to do a bit of review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far. I like the current section layout. The "read more" tags are pretty useful.
Where are we going to put the currently available hooks?
So the docs I'd written on hooks thus far were more like API docs. They jump right into the finer details of how the hooks work. As a result, I put them in the "Reference Material" section under "Hooks API". The broader plan is to progressively introduce hooks in the "Adding Interactivity" and "Managing State" sections through narratively driven explanations. The first place a reader would see a hook is under "Adding Interactivity" and "Components With State". |
eb8407f
to
9f10f80
Compare
@Archmonger here's the next build. At this point I've covered everything that the current docs do and more. I plan to mark which sections are still under construction, but in general I think this is really close to being ready to merge: build.zip |
I'll review this weekend. |
[16:46:30] [snowpack] ! building files...
[16:46:30] [snowpack] Build Result Error: There was a problem with a file build result.
[16:46:30] [snowpack] Package "idom-app-react" not found. Have you installed it?
[16:46:30] [snowpack] Error: Package "idom-app-react" not found. Have you installed it? |
I think you might need to update your version of NPM: |
792344c
to
dc7d1b7
Compare
dc7d1b7
to
4b523ae
Compare
Shoot. Never mind, I broke something in that last commit. I also amended the commit so that might prevent a simple |
Currently working on my review, not sure if the force push breaks that though. |
Should be ok. It was a minor fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, still need to handle the "Read More" pages for every tab below "Responding to Events". Will try to find time tomorrow after work to complete.
So far everything I've read has been a huge leap forward in both user friendliness and technical detail.
I believe there's some additional detail needed for the "Custom Components" section. Currently is too barebones. Needs additional info to guide users through going from start to finish,
docs/source/index.rst
Outdated
|
||
IDOM is a Python package for making user interfaces (UI). These interfaces are built | ||
from small elements of functionality like buttons text and images. IDOM allows you to | ||
combine these elements into reusable, nestable :ref:`"components" <your first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"nestable" is too verbose here, can be removed
Section 2: Running IDOM | ||
----------------------- | ||
|
||
Once you've :ref:`installed IDOM <Installing IDOM>`. The simplest way to run IDOM is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add a note here to let users know Django IDOM has different starting instructions than the rest.
Section 1: Installing IDOM | ||
-------------------------- | ||
|
||
The next fastest option is to install IDOM with ``pip``: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add a note here to let users know Django IDOM has different installation instructions than the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure this is the best place for that note since this section is really just helping the user try out IDOM as quickly and easily as possible. If the user is interested in learning more, they can following the "Read More" link where they'll see different ways to install IDOM. Alternatively they could search for the term "django" to find out the same information. With that said, perhaps there could be a section under "Running IDOM" (not just installation) to ensure users find this information.
d9c618f
to
4c5659d
Compare
1fa3ede
to
9b3424b
Compare
9b3424b
to
a8d308f
Compare
@Archmonger I think I'm going to merge this. We can make incremental improvements from there. |
I'll leave the residual review on this PR so we'll know what needs to get done on the incremental updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final review.
``UnboundLocalError`` error. It turns out that in this case, the ``index = index + 1`` | ||
statement is similar to `trying to set global variables | ||
<https://stackoverflow.com/questions/9264763/dont-understand-why-unboundlocalerror-occurs-closure>`__. | ||
Tehcnically there's a way to `fix this error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tehcnically
-> Technically
We've just now told IDOM that we want to update the state of our ``Gallery`` and | ||
that it needs to be re-rendered. More specifically, we are incrementing its | ||
``index``, and once ``Gallery`` re-renders the index *will* be ``1``. | ||
Importantly, at this point, **the value of ``index`` is still ``0``**! This will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index
is still 0
not properly rendering as a code block
- ``context`` can send events back to the server and load "import sources" | ||
(like a custom component module). | ||
|
||
- ``type``is a named export of the current module, or a string (e.g. ``"div"``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole line is improperly rendered as a code block
@@ -181,7 +44,7 @@ where you can then load any of its exports: | |||
|
|||
|
|||
Distributing Javascript via PyPI_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this section there is a misspelling of applciation
->application
, but it is not a new code modification so I cannot comment on it.
Examples | ||
======== | ||
|
||
Gallery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename this to "Code Examples".
Gallery makes it hard to know what this tab is supposed to be without clicking on it.
Life Cycle Hooks | ||
================ | ||
========= | ||
Hooks API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LAZY INITIAL STATE
section in here is important enough to move up to the Managing State
tab whenever that gets developed.
@@ -1,5 +1,5 @@ | |||
API Reference | |||
============= | |||
User API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly this tab is still impossible to read since it's one giant page. Definitely need to find a way to break this up into smaller sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At my job I got something nice working. Still one page, but it had nested sections for each module and package. Will port that over at some point.
This PR aims to re-organize existing documentation as well as create an outline for new documentation.
closes: #512, #442, #444, #445, #447, #448, #449